fix(graph): key IMPORTS edge dedup on local_name, not just source/target#770
fix(graph): key IMPORTS edge dedup on local_name, not just source/target#770apappas1129 wants to merge 2 commits into
Conversation
Two named imports from the same specifier resolve to the same (source_file, target_file) pair, so cbm_gbuf_insert_edge's dedup key of (source_id, target_id, type) collapsed them into one edge - the second import's properties_json silently replaced the first's, dropping whichever symbol lost the write race. This breaks more than "who imports X" queries: pass_calls.c, pass_usages.c, pass_semantic.c, and pass_lsp_cross.c all build their local_name -> resolved module QN maps for cross-file call resolution by parsing exactly one local_name out of each file's IMPORTS edges, so the dropped symbol's calls also failed to resolve cross-file. make_edge_key now folds local_name into the key for IMPORTS edges specifically, so two different symbols from the same specifier become two distinct edges while a re-inserted identical import (idempotent re-index) still dedups to one. Other edge types keep their original (source, target, type) key unchanged. Closes DeusData#768. Signed-off-by: Alexandros Pappas <11921291+apappas1129@users.noreply.github.com>
Signed-off-by: Alexandros Pappas <11921291+apappas1129@users.noreply.github.com>
DeusData
added
bug
|
Thanks for the IMPORTS dedup fix for #768. Triage: parsing/import graph correctness. Review will focus on whether edge identity including |
|
Thank you — the diagnosis is exactly right (main keys IMPORTS dedup on (src,tgt,type) and silently replaces properties, dropping sibling imports and breaking downstream local_name consumers), and your gbuf test is a genuine red-first guard at that tier. One blocking completeness issue we found while verifying end-to-end: the edge-uniqueness contract lives in three places, and this PR changes only one. Both persistence paths still declare UNIQUE(source_id, target_id, type) — the runtime store (store.c DDL + the ON CONFLICT upsert) and the raw dump writer (which emits that DDL and hand-builds the unique autoindex mechanically from all edges). With this change alone, any repo containing import { A, B } from './x' produces a DB that violates its own declared unique constraint — PRAGMA integrity_check reports non-unique index entries, and later upserts against that pair have ambiguous semantics. (CI stayed green only because no fixture has a multi-symbol import.) Could you extend the PR to change the contract coherently in all three places — e.g. include local_name in IMPORTS uniqueness or redefine the index — plus one end-to-end test: index a two-symbol import, dump, run integrity_check, query both edges back? Also worth bumping EDGE_KEY_BUF or documenting the truncation behavior for very long local names. With that, this is a clean merge — thanks again for the excellent issue write-up. Update: to keep momentum on the bug backlog, we're going to carry the changes above over the line ourselves shortly — a distilled follow-up on current main implementing the notes in this thread, with you credited as |
|
Thank you — your diagnosis was exactly right (dedup keyed on (src,tgt,type) silently replacing properties and dropping sibling imports), and your gbuf test was a genuine red-first guard at that tier. While verifying end-to-end we found the uniqueness contract lives in three places (graph buffer key, the store's UNIQUE DDL + upsert conflict target, and the raw dump writer's hand-built autoindex) — changing only one ships DBs that violate their own declared constraint, so we carried your fix over the line with the other two sites co-updated, a schema-compat probe for older DBs, and an end-to-end integrity_check test, crediting you as co-author. Merged as 541ff61 (PR #814), closing #768. Closing this in favor of the distill — your issue write-ups on this series (#767, #768, #730) have been a genuine pleasure to work with. Thanks again! |
…d dump Distills PR DeusData#770 (graph-buffer dedup key) and completes the edge-uniqueness contract it left partial: the contract lives in THREE places, and changing only the buffer ships databases that violate their own UNIQUE constraint (PRAGMA integrity_check: non-unique entry in sqlite_autoindex_edges_1). A single 'import { A, B } from ./lib' produced ONE IMPORTS edge: the graph buffer dedups edges on (source_id, target_id, type) and merge-replaces properties on collision, so the second symbol silently overwrote the first. pass_calls.c, pass_usages.c, pass_semantic.c and pass_lsp_cross.c parse one local_name per IMPORTS edge for cross-file resolution, so the dropped symbol's calls failed to resolve too — not just "who imports X" queries. Changes, kept in sync across all three sites: - graph_buffer.c (from DeusData#770): make_edge_key() folds local_name into the dedup key for IMPORTS edges only; all three key call-sites updated. Hardened beyond DeusData#770: EDGE_KEY_BUF bumped to 256 and oversized local_names are re-keyed with an FNV-1a hash of the full name instead of being silently truncated (two long names sharing a prefix must not collide back into one edge). - store.c: edges gains local_name_gen, a VIRTUAL generated column (IMPORTS -> coalesce(json_extract(properties,'$.local_name'),''), else '' — NOT NULL because NULLs never conflict in a UNIQUE index); uniqueness widened to UNIQUE(source_id, target_id, type, local_name_gen) and the insert upsert's conflict target matches. init_schema probes pre-DeusData#768 DBs (no local_name_gen) and fails the open: SQLite cannot ALTER a table constraint in place, and an unopenable DB already takes the existing repair path — full index deletes + rebuilds, artifact import refuses and falls back to a reindex. Read-only query opens skip init_schema and keep working. - sqlite_writer.c/.h: dump DDL matches the widened schema; the hand-built sqlite_autoindex_edges_1 comparator and entry builder include the local_name column; CBMDumpEdge carries local_name extracted via real JSON parsing (yyjson) so index entries match json_extract's unescaped values exactly, per the idx_edges_url_path precedent. - artifact.h: CBM_ARTIFACT_SCHEMA_VERSION 1 -> 2 so old binaries refuse artifacts carrying the widened schema (their 3-column conflict target can no longer prepare against it). Tests (reproduce-first, all red on the unfixed code): - gbuf tier: multi-symbol import -> 2 IMPORTS edges; long-local_name truncation guard. - store tier: distinct local_name coexists as 2 rows, same local_name still upserts, non-IMPORTS dedup unchanged. - writer tier: dumped DB passes integrity_check with 2 sibling imports and exposes matching local_name_gen values. - end-to-end: TS fixture through the real pipeline -> 2 queryable IMPORTS edges AND integrity_check ok; with only the buffer half of the fix this test still fails on integrity_check, proving the schema half is required. Closes DeusData#768. Co-authored-by: Alexandros Pappas <11921291+apappas1129@users.noreply.github.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
2 participants
What does this PR do?
Closes #768: a single
import { A, B } from './lib'statement produced exactlyone
IMPORTSedge instead of two — whichever symbol got written secondsilently overwrote the first.
cbm_gbuf_insert_edge()(src/graph_buffer/graph_buffer.c) dedups edges on(source_id, target_id, type)only. Two named imports from the same specifierresolve to the same
(source_file, target_file)pair, so the secondIMPORTSedge collides with the first on that key. On collision the functiondoesn't merge or reject — it replaces
properties_jsonoutright (theexisting
/* Merge properties (just replace for now) */comment suggeststhis was a known stub).
This has a bigger blast radius than "who imports X" queries:
pass_calls.c,pass_usages.c,pass_semantic.c, andpass_lsp_cross.call build theirlocal_name -> resolved module QNmaps for cross-file call resolution byparsing exactly one
local_nameout of eachIMPORTSedge's properties. Sothe dropped symbol's calls also failed to resolve cross-file, not just the
import edge itself.
make_edge_key()now foldslocal_nameinto the dedup key specifically forIMPORTSedges, so two different symbols imported from the same specifierbecome two distinct edges. A re-inserted identical import (e.g. an idempotent
re-index) still dedups to one edge. No changes were needed in any of the four
downstream consumers — they already iterate edges and parse one
local_nameper edge; they simply see both edges now that dedup stops collapsing them.
Other edge types (
CALLS,HTTP_CALLS,DEFINES, ...) keep their original(source, target, type)key, unchanged.Checklist
git commit -s)make -f Makefile.cbm test) — full suite greenexcept one pre-existing, unrelated RSS-ceiling test in
test_incremental.c(~2.6GB vs 2048MB limit) on this machinemake -f Makefile.cbm lint-ci) — could not verifylocally, cppcheck/clang-format are not installed in this
environment; relying on CI
gbuf_imports_multi_symbol_dedupin
tests/test_graph_buffer.c, asserting two same-target imports withdifferent
local_nameproduce distinct edges while a repeatedidentical import still dedups to one